HTTP/3: Eliminate near-zero "wait for connection" telemetry signals#114083
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the "wait for connection" telemetry logic for HTTP/3, ensuring such telemetry is only emitted for new connections where OpenOutboundStreamAsync() is expected to wait.
- Updated test cases in MetricsTest.cs and DiagnosticsTests.cs to reflect the new telemetry behavior.
- Refactored Http3Connection.SendAsync and associated telemetry tracking via a new WaitForHttp3ConnectionActivity structure.
- Adjusted HttpConnectionPool.Http3.cs to integrate the new telemetry mechanism and removed obsolete timestamp parameters.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs | Added a test to ensure queue time is measured only for the first connection. |
| src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs | Updated expected telemetry count in diagnostics tests. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3Connection.cs | Refactored SendAsync to use WaitForHttp3ConnectionActivity and updated telemetry logic. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionWaiter.cs | Added a comment to clarify custom telemetry behavior for HTTP/3 connections. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/HttpConnectionPool.Http3.cs | Switched to the new telemetry mechanism and removed obsolete parameters. |
| src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectionPool/ConnectionSetupDistributedTracing.cs | Exposed a new method to check for telemetry listeners. |
This comment was marked as outdated.
This comment was marked as outdated.
…pleHttp3Connections is false
ManickaP
left a comment
There was a problem hiding this comment.
LGTM, thanks!
If you don't merge it byt the end of the day because you're already OOF, I'll merge it myself.
|
@pavelsavara Seems like your new test in https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/tests/TrimmingTests/MetricsHandlerTrimmedTest.cs is failing on this PR. Would be able to help with resolving it? |
Probably the new code keeps the metrics classes alive. You can run the test and see which dependencies are doing it by looking at I suspect that the new code in |
|
|
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
We should only emit wait for connection activities, metrics and events when (1) there is no HTTP/3 connection (2)
OpenOutboundStreamAsync()is expected to wait for available streams. When there is a usable connection in the pool, emitting near-zero spans is not consistent with telemetry for other HTTP versions and adds noise.Fixes #104783.